-
Notifications
You must be signed in to change notification settings - Fork 89
Make releaseNumberSDK configurable parameter in checkVersions.yml #3418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: laeubi <[email protected]>
Co-authored-by: laeubi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks useful as it at least allows to configure things, having a property in the pom also seems flexible enough for most use-cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking the input is probably not necessary, see my comment below.
Besides that I think this is fine.
| # Validate property name to prevent injection | ||
| propertyName="${{ inputs.currentStreamVersionProperty }}" | ||
| if [[ ! "$propertyName" =~ ^[a-zA-Z0-9._-]+$ ]]; then | ||
| echo "::error::Invalid property name: $propertyName. Must contain only alphanumeric characters, dots, hyphens, and underscores." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is necessary as this job doesn't have any permissions:
| permissions: {} # all none |
and with extra-setup-command we have another argument that is also executed as it is.
So this extra check does not really buy us extra safety and therefore I suggest to keep it simple and remove it.
|
@laeubi how should we proceed here? |
Problem
The
checkVersions.ymlworkflow currently hardcodes the Maven property namereleaseNumberSDKfor determining the stream version used in commit messages. Different projects may need to use different Maven property names for this purpose, making the workflow less flexible.Solution
This PR adds a new optional input parameter
currentStreamVersionPropertyto thecheckVersions.ymlworkflow, allowing callers to specify which Maven property to use for determining the stream version.Changes
currentStreamVersionPropertywith a default value of'releaseNumberSDK'to maintain backward compatibility^[a-zA-Z0-9._-]+$) to prevent potential Maven expression injection attacksreleaseNumberSDKto use the configurable parameterBackward Compatibility
This change is fully backward compatible. Existing workflows that call
checkVersions.yml(such aspr-checks.yml) will continue to work without any modifications, as the parameter has a sensible default value.Usage Examples
Default behavior (uses
releaseNumberSDK):Custom property:
Security
Added input validation to prevent injection attacks. The parameter must contain only alphanumeric characters, dots, hyphens, and underscores. Any invalid input will result in a clear error message and workflow failure.
Fixes the issue by making the stream version property configurable while maintaining security and backward compatibility.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.